Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Validator infrastructure #27961

Closed
wants to merge 10 commits into from
Closed

Add Validator infrastructure #27961

wants to merge 10 commits into from

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Jul 14, 2021 •
edited
Loading

Originally this PR tried to fix the validation of URLs, but the scope of this
changed to instead provide a generic interface for validating user input,
configs, ... The theming API was ported to it as example and to fix the
original bug.

The API is based on Symfony Validator component but the implementation
is simplified a lot since we don't support (yet) validating class with annotation
and other more complicated (and useful) stuff.

The filter_var function is unfortunately not perfect and doesn't support
domain with unicode as well as url with underscores. Replace usage with
a regex.

See https://bugs.php.net/search.php?cmd=display&search_for=FILTER_VALIDATE_URL

Closes #27906

Signed-off-by: Carl Schwan [email protected]

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Jul 14, 2021
@szaimen szaimen added this to the Nextcloud 23 milestone Jul 14, 2021
@juliusknorr
Copy link
Member

Mind to squash to a single commit? :)

@blizzz
Copy link
Member

blizzz commented Jul 14, 2021

CI is unhappy tho

@CarlSchwan
Copy link
Member Author

It's a bit weird that the ci report a failure for continuous-integration/drone/pr but in https://drone.nextcloud.com/nextcloud/server/7311 everything is green

@juliusknorr
Copy link
Member

It's a bit weird that the ci report a failure for continuous-integration/drone/pr but in drone.nextcloud.com/nextcloud/server/7311 everything is green

What about https://drone.nextcloud.com/nextcloud/server/7311/9/4 😉


There were 4 failures:

1) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetSuccess with data set #1 ('url', 'https://nextcloud.com/aaaaaaa...aaaaaa', 'Saved')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'Saved'
-            'serverCssUrl' => '/nextcloudWebroot/core/css/so...s.scss'
+            'message' => 'The given web address is not a valid URL'
         )
-        'status' => 'success'
+        'status' => 'error'
     )
     'headers' => Array (...)
     'cookies' => Array ()
-    'status' => 200
+    'status' => 400
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\EmptyContentSecurityPolicy Object (...)

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:168

2) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetSuccess with data set #6 ('imprintUrl', 'https://nextcloud.com/aaaaaaa...aaaaaa', 'Saved')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'Saved'
-            'serverCssUrl' => '/nextcloudWebroot/core/css/so...s.scss'
+            'message' => 'The given legal notice address is not a valid URL'
         )
-        'status' => 'success'
+        'status' => 'error'
     )
     'headers' => Array (...)
     'cookies' => Array ()
-    'status' => 200
+    'status' => 400
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\EmptyContentSecurityPolicy Object (...)

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:168

3) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetSuccess with data set #7 ('privacyUrl', 'https://nextcloud.com/aaaaaaa...aaaaaa', 'Saved')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'Saved'
-            'serverCssUrl' => '/nextcloudWebroot/core/css/so...s.scss'
+            'message' => 'The given privacy policy address is not a valid URL'
         )
-        'status' => 'success'
+        'status' => 'error'
     )
     'headers' => Array (...)
     'cookies' => Array ()
-    'status' => 200
+    'status' => 400
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\EmptyContentSecurityPolicy Object (...)

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:168

4) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetError with data set #1 ('url', 'http://example.com/aaaaaaaaaa...aaaaaa', 'The given web address is too long')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'The given web address is too long'
+            'message' => 'The given web address is not a valid URL'
         )
         'status' => 'error'
     )

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:217

--

@CarlSchwan
Copy link
Member Author

It seems idn_to_ascii is kinda evil too and just abort when the URL is too long :(

A better solution would be either to just copy the UrlValidator::validate from symfony/validator or just depends on symfony/validator and use it for all kind of validations in Nextcloud (URL, email, date ...). For reference, this is the method: https://github.com/symfony/validator/blob/14337bdf9e2e0b2e3385c9e90f13325f0c95a4f9/Constraints/UrlValidator.php#L24

@tcitworld
Copy link
Member

It seems idn_to_ascii is kinda evil too and just abort when the URL is too long :(

That's probably because it's made to handle domains, not full URIs. You could extract the hostname with parse_url, convert it, then substr_replace it in the URL.

But using the regex from SF Validator makes sense too.

julien-nc
julien-nc requested changes Jul 20, 2021
View reviewed changes
lib/public/Util.php Outdated Show resolved Hide resolved
julien-nc
julien-nc previously requested changes Jul 20, 2021
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that, code looks good. Didn't try it though.

apps/oauth2/lib/Controller/SettingsController.php Outdated Show resolved Hide resolved
apps/theming/lib/ThemingDefaults.php Outdated Show resolved Hide resolved
lib/private/Setup.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch 4 times, most recently from e62819d to 1fd5538 Compare August 6, 2021 13:10
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

What is missing here?

@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
Merged
@CarlSchwan CarlSchwan added the bug label Oct 21, 2021
@blizzz blizzz mentioned this pull request Sep 9, 2022
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
This was referenced Sep 20, 2022
Closed
Merged
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
@blizzz blizzz mentioned this pull request Mar 7, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 17, 2023
@szaimen
Copy link
Contributor

szaimen commented Apr 17, 2023

conflicts :/

This was referenced May 3, 2023
Merged
Closed
Merged
@blizzz blizzz mentioned this pull request May 17, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
This was referenced Nov 6, 2023
Merged
Merged
This was referenced Nov 14, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv closed this Mar 10, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Mar 10, 2024
@skjnldsv skjnldsv deleted the work/carl/idn-url-theming branch March 14, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc left review comments

@tcitworld tcitworld tcitworld left review comments

@artonge artonge artonge approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@julien-nc julien-nc Awaiting requested review from julien-nc

@szaimen szaimen Awaiting requested review from szaimen

@blizzz blizzz Awaiting requested review from blizzz

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees
No one assigned
Labels
2. developing Work in progress bug pending documentation This pull request needs an associated documentation update technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IDN domain name not allowed in /settings/admin/theming
10 participants
@CarlSchwan @juliusknorr @blizzz @tcitworld @szaimen @skjnldsv @PVince81 @artonge @julien-nc @come-nc